Bottom sheet lists#98
Conversation
| // todo: 48 and 36 should be input heights and we should make them dynamically themed | ||
| const paddingVertical = label | ||
| ? (48 - (styles.headerTextStyle.lineHeight! + styles.value.lineHeight!)) / 2 | ||
| : (36 - styles.value.lineHeight!) / 2 |
There was a problem hiding this comment.
Ik zou dit liever dynamisch maken zodat de input iets groter is in Observation dan in ObsIdentify. Voor nu heb ik het zo gelaten omdat het al meer dan genoeg werk voor 1 story was.
There was a problem hiding this comment.
Dat kan door paddingVertical niet af te laten hangen van de lineHeight, maar een fixed value te gebruiken.
Aan de andere kant, Karel heeft ideeën over hoe groot elementen moeten zijn (veelvoud van 8 of 16) om te zorgen voor een evenwichtige en consistente vulling van de schermen. Dan zou Karel dus moeten aangeven hoe dit in beide apps eruit moet zien, en kunnen wij de implementatie van de UI daarop baseren.
| const styles = useStyles(createStyles) | ||
|
|
||
| const iconMarginRight = subLabel ? theme.margin.common : theme.margin.half | ||
| const containerPaddingVertical = subLabel ? 7 : 13 |
There was a problem hiding this comment.
Deze hard-coded padding is niet zo mooi maar ik kon geen betere manier verzinnen.
| // todo: 48 and 36 should be input heights and we should make them dynamically themed | ||
| const paddingVertical = label | ||
| ? (48 - (styles.headerTextStyle.lineHeight! + styles.value.lineHeight!)) / 2 | ||
| : (36 - styles.value.lineHeight!) / 2 |
There was a problem hiding this comment.
Dat kan door paddingVertical niet af te laten hangen van de lineHeight, maar een fixed value te gebruiken.
Aan de andere kant, Karel heeft ideeën over hoe groot elementen moeten zijn (veelvoud van 8 of 16) om te zorgen voor een evenwichtige en consistente vulling van de schermen. Dan zou Karel dus moeten aangeven hoe dit in beide apps eruit moet zien, en kunnen wij de implementatie van de UI daarop baseren.
| ) | ||
| } | ||
|
|
||
| // todo: dit werd geexporteerd als een observer |
There was a problem hiding this comment.
Graag TODO in hoofdletters, en de tekst in het Engels. Wat moet hier nog gebeuren?
There was a problem hiding this comment.
Ah, ik denk dat die weg kan. Het viel me op dat er eerder een observer was gebruikt en daar had ik een notitie van gemaakt voor mezelf. Volgens mij is het geen probleem dus ik kan het weghalen.
| import { Icon } from '@observation.org/react-native-components' | ||
| import { type Theme, useStyles, useTheme } from '@observation.org/react-native-components/theme' |
There was a problem hiding this comment.
Import paden zijn niet correct, moeten interne paden zijn.
| } | ||
|
|
||
| const ListItem = ({ icon, onPress, label, subLabel, extraSubLabel, selected = false }: Props) => { | ||
| Log.trace('ListItem', onPress) |
| test('Rendering', () => { | ||
| // WHEN | ||
| const { toJSON } = render(<ListItem onPress={onPress} label="Read this!" />) | ||
|
|
||
| // THEN | ||
| expect(toJSON()).toMatchSnapshot() | ||
| }) |
There was a problem hiding this comment.
Deze kan weg, wordt herhaald in de volgende unit test.
| headerTextStyle: { | ||
| ...theme.font.extraSmall, | ||
| lineHeight: theme.font.extraSmall.fontSize, | ||
| letterSpacing: 0.3, // todo: should this be dynamic? |
There was a problem hiding this comment.
Property letterSpacing is in points. Het font is extra small, voor ObsIdentify is dat 10pt. Een letterSpacing van 0.3pt geeft dus 3% meer ruimte. Je kunt dit vrij simpel dynamisch maken:
| letterSpacing: 0.3, // todo: should this be dynamic? | |
| letterSpacing: 0.03 * theme.font.extraSmall.fontSize |
SjaakSchilperoort
left a comment
SjaakSchilperoort
left a comment
There was a problem hiding this comment.
Paar kleine dingen nog, verder 👌🏻
| const theme = useTheme() | ||
| const styles = useStyles(createStyles) | ||
|
|
||
| // todo: 48 and 36 should be input heights and we should make them dynamically themed |
There was a problem hiding this comment.
Het is fijn als VSCode de TODO's kan vinden, dus:
| // todo: 48 and 36 should be input heights and we should make them dynamically themed | |
| // TODO: 48 and 36 should be input heights and we should make them dynamically themed |
| import React from 'react' | ||
| import { StyleSheet, TouchableOpacity, View } from 'react-native' | ||
|
|
||
| import { Icon, IconProps } from '@observation.org/react-native-components' |
There was a problem hiding this comment.
Hier nog een onnodige dependency op zichzelf. Idem in de bijbehorende unit test
2 participants
Required to complete https://github.com/observation/obsidentify/issues/1248.
Five components that are used in the bottom sheet with search input can be shared between the apps. I'll leave some comments on little things that I found along the way.
We first need to merge this. Then we can rebase the related PRs in Observation and ObsIdentify.